Skip to content

use sds to fetch the client certificate for envoy-ratelimit TLS connections#5988

Merged
zirain merged 10 commits intoenvoyproxy:mainfrom
zhaohuabing:fix-gatewaynamespace-ratelimit
May 21, 2025
Merged

use sds to fetch the client certificate for envoy-ratelimit TLS connections#5988
zirain merged 10 commits intoenvoyproxy:mainfrom
zhaohuabing:fix-gatewaynamespace-ratelimit

Conversation

@zhaohuabing
Copy link
Member

@zhaohuabing zhaohuabing commented May 10, 2025 •
edited
Loading

This PR uses SDS to fetch the envoy client certificate, and enables Global ratelimit test for gatewaynamepace mode.

@zhaohuabing zhaohuabing requested a review from a team as a code owner May 10, 2025 02:31
@zhaohuabing zhaohuabing marked this pull request as draft May 10, 2025 02:31
@zhaohuabing zhaohuabing mentioned this pull request May 10, 2025
Closed
@zhaohuabing zhaohuabing force-pushed the fix-gatewaynamespace-ratelimit branch 5 times, most recently from 2caf7ca to f7233d0 Compare May 10, 2025 04:13
@codecov
Copy link

codecov bot commented May 10, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 82.05128% with 28 lines in your changes missing coverage. Please review.

Project coverage is 70.47%. Comparing base (452651b) to head (0e27f32).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
internal/xds/translator/globalresources.go 80.00% 11 Missing and 5 partials ⚠️
internal/provider/kubernetes/controller.go 76.00% 6 Missing ⚠️
internal/provider/kubernetes/predicates.go 66.66% 2 Missing and 1 partial ⚠️
internal/xds/translator/translator.go 0.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5988      +/-   ##
==========================================
- Coverage   70.50%   70.47%   -0.04%     
==========================================
  Files         217      219       +2     
  Lines       36191    36315     +124     
==========================================
+ Hits        25517    25593      +76     
- Misses       9160     9199      +39     
- Partials     1514     1523       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

zhaohuabing
zhaohuabing commented May 10, 2025
View reviewed changes
internal/gatewayapi/testdata/backendtrafficpolicy-with-ratelimit.out.yaml Outdated
Copy link
Member Author

@zhaohuabing zhaohuabing May 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The serverCertificate IR Certificate struct yaml key should be renamed to certificate.

This rename also affects many existing test files. To make the review easier, I'll raise a follow-up PR to change this to limit the scope of this PR.

@zhaohuabing zhaohuabing marked this pull request as ready for review May 10, 2025 05:03
@zhaohuabing zhaohuabing requested a review from a team May 10, 2025 05:04
@zhaohuabing zhaohuabing force-pushed the fix-gatewaynamespace-ratelimit branch from 03c922b to d37254f Compare May 10, 2025 05:08
@zhaohuabing zhaohuabing marked this pull request as draft May 10, 2025 05:09
@zhaohuabing zhaohuabing force-pushed the fix-gatewaynamespace-ratelimit branch 2 times, most recently from bbb281b to 5b43ca5 Compare May 10, 2025 05:52
@zhaohuabing zhaohuabing changed the title use uds to fetch the client certificate for envoy-ratelimit TLS connections use sds to fetch the client certificate for envoy-ratelimit TLS connections May 10, 2025
@zhaohuabing zhaohuabing marked this pull request as ready for review May 10, 2025 05:58
@zhaohuabing zhaohuabing changed the title use sds to fetch the client certificate for envoy-ratelimit TLS connections gatewaynamespace: use sds to fetch the client certificate for envoy-ratelimit TLS connections May 10, 2025
@zhaohuabing zhaohuabing changed the title gatewaynamespace: use sds to fetch the client certificate for envoy-ratelimit TLS connections use sds to fetch the client certificate for envoy-ratelimit TLS connections May 10, 2025
@zhaohuabing zhaohuabing added this to the v1.4.0 milestone May 10, 2025
@arkodg
Copy link
Contributor

arkodg commented May 10, 2025

hey @zhaohuabing can you help understand why default mode implementation doesn't work in gateway namespace mode

@zhaohuabing
Copy link
Member Author

zhaohuabing commented May 10, 2025

hey @zhaohuabing can you help understand why default mode implementation doesn't work in gateway namespace mode

EG mounts the "envoy" secret to the envoy container, which holds the client cert used by envoy to establish TLS connections with the rate limit service. But this secret is not available in the the namespaces except the envoy-gateway-system namespace.

This PR loads the "envoy" secret from the envoy -gateway-system and push it to envoy through xDS.

@arkodg
Copy link
Contributor

arkodg commented May 10, 2025

Ah this is for communication b/w envoy and RL service, thanks for the clarification

@zirain
Copy link
Member

zirain commented May 10, 2025

we may need same thing for wasm cache service.

@zhaohuabing
Copy link
Member Author

zhaohuabing commented May 11, 2025 •
edited
Loading

we may need same thing for wasm cache service.

Yeah will try to figure out why wasm test fails and address it in a follow up pr.

arkodg
arkodg reviewed May 12, 2025
View reviewed changes
arkodg
arkodg reviewed May 12, 2025
View reviewed changes
arkodg
arkodg reviewed May 12, 2025
View reviewed changes
arkodg
arkodg reviewed May 12, 2025
View reviewed changes
@zhaohuabing zhaohuabing force-pushed the fix-gatewaynamespace-ratelimit branch 5 times, most recently from 2f49823 to 931361b Compare May 17, 2025 09:02
@zhaohuabing
only create envoy client cert when global rate limit is enabled
ce04a71 
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
@zhaohuabing zhaohuabing force-pushed the fix-gatewaynamespace-ratelimit branch from 931361b to ce04a71 Compare May 17, 2025 09:19
@zhaohuabing
Merge branch 'main' into fix-gatewaynamespace-ratelimit
d73acfe 
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
arkodg
arkodg reviewed May 19, 2025
View reviewed changes
arkodg
arkodg reviewed May 19, 2025
View reviewed changes
@zhaohuabing
address comment
61b91b9 
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
@zhaohuabing
Merge remote-tracking branch 'origin/main' into fix-gatewaynamespace-…
eb51aab 
…ratelimit
@zhaohuabing zhaohuabing requested a review from arkodg May 20, 2025 03:18
arkodg
arkodg reviewed May 20, 2025
View reviewed changes
@zhaohuabing
address comment
0e27f32 
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
@zhaohuabing zhaohuabing requested a review from arkodg May 21, 2025 00:41
arkodg
arkodg approved these changes May 21, 2025
View reviewed changes
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks !

@arkodg arkodg requested review from a team May 21, 2025 01:06
@zirain zirain merged commit 31a929c into envoyproxy:main May 21, 2025
27 checks passed
@zhaohuabing zhaohuabing deleted the fix-gatewaynamespace-ratelimit branch May 21, 2025 02:40
@BrewTestBot BrewTestBot mentioned this pull request Aug 8, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zirain zirain zirain approved these changes

@arkodg arkodg arkodg approved these changes

@cnvergence cnvergence Awaiting requested review from cnvergence

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

4 participants

@zhaohuabing @arkodg @zirain @cnvergence